feat: run scanner-selected conversation turns - #467

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Verdict: Worth merging

Good taste — The PR cleanly extends the existing run/dispatch infrastructure to support service-owned agent turns without introducing special-case spaghetti. The data structure additions (subject_source, conversation_turn, conversation_wake_agent) are minimal and well-scoped. Subject routing is now namespaced by source via the advisory lock digest and the partial index, preventing cross-integration key collisions. The dispatcher's concurrency gate correctly separates scanner runs (blocked from overlapping) from independent agent-turn runs (allowed to fan out).

Key observations

Migration (027): Cross-database compatible — uses generic SQLAlchemy types, conditional SQL for the backfill (json_extract for SQLite, ->> for PostgreSQL), and conditionally applies the partial index predicate for both dialects. The drop-and-recreate of ix_automation_runs_subject to include subject_source is correct.

Conversation ID persistence: Moving the conversation_id and sandbox_id persistence to before the turn execution (dispatcher.py:369-380) is the right call — follow-up turns from continue_conversation can route to the correct conversation as soon as it starts, rather than waiting for bundle completion.

Subject release flow: The success path does not set subject_released_at, which is consistent with existing conversation-run behavior. Follow-up events that find a COMPLETED run attempt delivery; if the runtime is gone (Docker released), delivery fails and the subject is released, falling back to a fresh run. For local runtime, the conversation persists and turns are delivered successfully. Both paths are correct.

Error path: On turn failure, the subject is released, the runtime context is released, and the run is marked FAILED with structured status detail. On success, release_context failures are logged but don't fail the run — conversation history is persisted outside the runtime, with TTL cleanup as fallback. This is pragmatic.

Runtime scope: _run_turn correctly passes runtime_conversation_id to RemoteWorkspace, ensuring agent tools execute in the provisioned Docker/local runtime rather than the host. The test test_service_owned_turn_keeps_the_runtime_scope verifies this.

Dependency: openhands-tools is added from the same first-party git ref as openhands-sdk, satisfying the PR's stated SDK dependency. First-party, same-org — no 7-day waiting period applies.

[RISK ASSESSMENT]

• Overall PR: LOW
  • New code path (agent-turn dispatch) is cleanly separated from the existing bundle-execution path via the conversation_turn is not None guard.
  • Migration is additive (nullable columns, index recreation) with a correct downgrade.
  • No new external attack surface — the turn text originates from the scanner script running inside the service's own sandbox, not from untrusted external input.
  • Test coverage spans poller fan-out, dispatch skip-bundle-and-release, runtime scope, subject isolation, and SQLite migration.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with thumbs up or thumbs down to give feedback.

Approval review

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Review: feat: run scanner-selected conversation turns

Taste Rating: 🟢 Good taste

This PR adds service-owned conversation turns — a run-scoped scanner can select independent agent tasks, each created as an explicit conversation-scoped child run. The design is clean: it reuses existing run utilities, subject routing, and backend provisioning, adding only what's new (the turn text, the namespaced source, and the synchronous turn execution path).

Key strengths

• Subject source namespacing is the right fix: including source in the advisory lock digest and the subject lookup query prevents opaque-key collisions across integrations. The migration backfill is correct and cross-database compatible.
• Dispatcher fan-out logic is well-reasoned: conversation-scoped runs fan out to capacity while run-scoped commands are prevented from overlapping for one definition. The or_() filter cleanly separates the two concerns.
• Early context persistence (conversation_id + sandbox_id before work starts) is the correct ordering — a follow-up event can find an in-flight conversation.
• Failure handling in the conversation-turn path is thorough: subject lock is released, runtime is released, and the run is marked FAILED with structured status detail.

Minor observations (non-blocking)

• _run_turn uses conversation.run(timeout=timeout) (blocking) while the existing _send_turn uses conversation.run(blocking=False). This is intentional — the service-owned turn needs to know completion to mark COMPLETED and release the runtime, whereas webhook turns rely on the run's own watchdog. The blocking call is correctly wrapped in asyncio.to_thread.
• test_conversation_turn_run_skips_bundle_and_releases_runtime mocks run_conversation_turn (the unit under test). This is acceptable here since exercising the real path requires a live agent server. The test still verifies the dispatch flow, release ordering, and terminal status.

No material issues found

The migration is cross-database compatible (SQLite/PostgreSQL branches). The index recreation adds subject_source correctly. The conversation_id property in ConversationBackend properly checks the explicit run-level ID before falling back to the derived one. Subject release on failure prevents orphaned subjects.

[RISK ASSESSMENT]

• Overall PR: 🟢 LOW
  • The change adds a new execution path without modifying existing dispatch behavior for non-turn runs (the conversation_turn is not None check short-circuits before the bundle execution section).
  • The batch_size change from min(batch_size, 1, conversation_capacity) to min(batch_size, conversation_capacity) is the only behavioral change to existing polling, and it's covered by the fan-out test.
  • Migration is additive (new nullable columns) with a correct backfill.

VERDICT: ✅ Worth merging

KEY INSIGHT: The design correctly separates run-scoped overlap prevention from conversation-scoped fan-out, using execution_scope as the discriminator in the dispatcher's query filter.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Review: feat: run scanner-selected conversation turns

Taste Rating: Good taste

This PR introduces conversation-turn runs: a scanner (run-scoped automation) can select multiple independent agent tasks and dispatch each as an explicit conversation-scoped child run, without the scanner itself becoming a persistent conversation. The design is clean and leverages existing infrastructure (run utilities, ConversationBackend, subject locking, watchdog).

Key observations

Subject routing namespaced by source -- Adding subject_source to the advisory lock hash and the _lock_subject_run query is the right fix. Without it, equal opaque keys from different integrations (e.g. a GitHub issue number and a Slack thread id) would collide on the same advisory lock and subject lookup. The migration backfills existing rows from the automation trigger, and the index is recreated with subject_source included.

Conversation turn dispatch -- The dispatcher correctly differentiates conversation-turn runs from bundle runs: it persists the provisioned conversation_id and sandbox_id before work begins (so follow-up events can find in-flight work), runs the turn synchronously via run_conversation_turn, releases the runtime, and marks the run terminal. The error path properly releases the subject lock and runtime before marking the run FAILED.

Fan-out with overlap prevention -- The polling logic change from min(batch_size, 1, conversation_capacity) to min(batch_size, conversation_capacity) allows multiple conversation runs to dispatch per poll cycle, while the new active_run_automations check prevents overlapping run-scoped commands for one definition. Conversation-scoped work fans out independently to the configured capacity. This matches the PR intent.

_run_turn runtime scope -- The service-owned turn correctly passes runtime_conversation_id from the execution context to RemoteWorkspace, ensuring the agent tools stay in the conversation's selected local/Docker runtime rather than provisioning a new one.

Migration -- Cross-database compatible (generic SQLAlchemy types, conditional SQL for SQLite vs PostgreSQL, partial index recreation with both postgresql_where and sqlite_where). Downgrade restores the original index shape.

Minor note (non-blocking)

The assert run.conversation_id is not None on dispatcher.py:389 uses a Python assert for an invariant that holds (conversation turn runs always set conversation_id via conversation_id_for at creation). This is consistent with existing assert usage in the codebase (e.g. ingest.py:184, preset_router.py:868), so it follows convention. A plain if guard would be more robust under -O, but this is not a practical concern given the invariant.

Testing

The tests are meaningful: test_local_poller_fans_out_conversations_from_one_automation exercises the real polling logic against SQLite and verifies fan-out behavior. test_conversation_turn_run_skips_bundle_and_releases_runtime verifies the dispatch path skips the bundle and releases the runtime. test_service_owned_turn_keeps_the_runtime_scope verifies the runtime conversation id is propagated to the workspace.

Risk Assessment

[RISK ASSESSMENT]

• Overall PR: LOW

The change is well-scoped, builds on established patterns (subject locking, run lifecycle, backend abstraction), and the migration is cross-database compatible. The conversation-turn path is cleanly separated from the bundle execution path. No security concerns: conversation turn text is treated as data (same as existing event payloads), subject routing is properly namespaced, and runtime cleanup is handled on both success and failure paths.

Verdict: Worth merging

Key insight: Namespacing subject routing by source eliminates a real cross-integration collision risk, and the conversation-turn dispatch path cleanly separates service-owned turns from bundle execution while reusing the existing run lifecycle and subject-locking infrastructure.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with thumbs up or thumbs down to give feedback.